fix(tests): WebGL save/restore assertions for the actual 4x4 matrix layout (#1481)#1483
Merged
Merged
Conversation
…ayout (#1481) Three tests in `webgl_save_restore.spec.js` had been hardcoding indices that assumed `Matrix3d.val` was a 3x3 column-major matrix (val[4] = 1 for identity). It's actually a 4x4 column-major matrix — 16 floats — and the 4x4 identity has 1s at val[0], val[5], val[10], val[15], not val[0] and val[4]. The save/restore mechanism in RenderState was correct all along; the assertions were just expressing the wrong shape. Replaced the three sites with `expect(renderer.currentTransform.isIdentity()).toBe(true)` — layout- agnostic and says what we actually mean. Also converted all 13 `if (!isWebGL) { return; }` skips to `requireWebGL(ctx)` → `ctx.skip("WebGL not available")`. The bare `return` registered as `passed` in the reporter — combined with `failIfMajorPerformanceCaveat: true` in the spec setup, chromium headless without GPU flags consistently fell back to Canvas, isWebGL became false, every test returned immediately, the reporter showed "13 passed". The matrix-index assertions had been broken since commit `af34dd341` (Mar 2026) and the silent skips hid that from CI for months. `ctx.skip()` surfaces "13 skipped" in the reporter, so a future regression of the same shape would actually be visible. `failIfMajorPerformanceCaveat` flipped from `true` to `false` in the `beforeAll` so local devs / GPU-backed runners actually run these tests under software WebGL (chromium's SwiftShader backend) instead of falling back to Canvas and skipping silently. 3969 / 13 skipped / 0 failed in the default chromium headless env (the 13 webgl_save_restore tests now correctly show as skipped). 13 / 0 / 0 under SwiftShader (verified locally with the launch flags from the post-mortem). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrectly-written WebGL save/restore tests by removing brittle assumptions about Matrix3d.val’s internal layout (it’s a 4×4 / 16-float matrix) and by ensuring WebGL-dependent tests report as skipped (not falsely “passed”) when WebGL isn’t available in the test environment.
Changes:
- Replace hardcoded matrix-slot assertions with
renderer.currentTransform.isIdentity()in the three affected tests. - Replace the “silent-pass”
if (!isWebGL) return;pattern with arequireWebGL(ctx)helper that callsctx.skip(...). - Set
failIfMajorPerformanceCaveat: falseduring test initialization to better allow software WebGL contexts in constrained environments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1481.
Summary
Three tests in `webgl_save_restore.spec.js` were broken since they were committed in `af34dd341` (Mar 2026) — they hardcoded matrix-slot indices that assumed `Matrix3d.val` was a 3×3 column-major matrix. It's actually 4×4 / 16 floats. The save/restore mechanism in `RenderState` is correct; the tests' assertions were just expressing the wrong shape.
For a 4×4 identity:
The failing tests asserted `val[4] = 1` — which would be correct for a 3×3 identity but is 0 for a 4×4.
Why this slipped through CI for months
The spec used the silent-skip anti-pattern:
```js
it("name", () => {
if (!isWebGL) {
return; // ← registers as PASSED, not skipped
}
// assertions...
});
```
Combined with `failIfMajorPerformanceCaveat: true` in the spec's `beforeAll`, chromium headless without GPU flags fails to create a WebGL context → falls back to Canvas → `isWebGL = false` → every test returns immediately → reporter shows "13 passed".
The broken assertions never ran a single time in CI. Three years of green ticks for tests that weren't actually testing anything.
If the author had used `ctx.skip(...)` the reporter would have shown "13 skipped" and someone would have asked "why are our WebGL tests skipped?" months ago.
Changes
Verification
Follow-up (separate from this PR)
`webgl_pipeline_adversarial.spec.js` has the same silent-skip anti-pattern at 22 sites. Worth a similar pass — but the fuzz test in that spec needs separate stabilization work under software WebGL, so it's intentionally not bundled here.
🤖 Generated with Claude Code